refactor: pre-M4 protocol cleanup — ACP path unification, legacy removal, type safety - #147
Conversation
Replace all _run_stream_once references in tests with _stream_events, which is the public entry point that will survive T2's refactor (it will delegate to ACPTurn.execute internally). 10 test files updated: - 5 with active mock updates (test_run_stream_direct_gating, test_auto_resume_message_redflag, test_session_scoped_consumer, test_session_pool_input_provider, test_session_lifecycle) - 5 with comment/name-only updates (test_run_lifecycle, test_base_agent_run_v2, test_inject_prompt_cross_task, test_capability_hooks_standalone, test_cancelled_message) Verification: - grep -rn '_run_stream_once' tests/ returns 0 matches - 65 tests pass across all modified files - V10 snapshot tests pass (2/2)
…nager.add_transport()
Migrate all HostContext.pool access sites to use MessageNode._agent_pool directly, then remove the pool field from HostContext and the pool=self argument from AgentPool.get_context(). Source changes: - context.py: Remove pool field, AgentPool import, and Any import - pool.py: Remove pool=self from HostContext constructor - base_team.py: Use _agent_pool instead of host_context.pool (2 sites) - state.py: Use agent._agent_pool instead of _ctx.pool - agent_routes.py: Use state.agent._agent_pool instead of ctx.pool - acp_agent.py: Use default_agent._agent_pool instead of ctx.pool (4 sites) - native_agent/agent.py: Use self._agent_pool instead of ctx.pool (2 sites) Test changes: - Remove pool back-reference tests from test_pool_get_context.py and test_context.py - Update conftest.py and 10+ test files to set _agent_pool instead of pool.pool - Remove get_context mock from test_team_member_skills.py - Remove ctx.pool from test_factory.py
…re cluster - Update test_steer_direct_channel to verify deliver_feedback returns False - Update test_session_migration for publishes_to_event_bus property - Append T15 learnings to notepad
- Remove unused message_history param from ACPTurn.__init__ (dead code) - Cast self._api to ACPClientProtocol in acp_agent.py call sites - Fix status annotation from str to SessionStatusType in event_processor.py - Clean up stale # type: ignore comments - Update tests to match ACPTurn constructor changes
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive cleanup of technical debt across the ACP server, OpenCode server, and shared orchestration infrastructure, establishing a stable baseline for M4 (multi-config) development. Key changes include unifying ACP execution paths through Turn.execute(), refactoring RunHandle.start() into composable sub-methods, removing the RunStatus enum in favor of RunState and RunOutcome, and cleaning up legacy MCP lifecycle state management. I have addressed the reviewer's feedback regarding the Gantt chart dates and the encapsulation of _agent_pool access.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| for agent in self.iter_agents(): | ||
| pool = agent.host_context.pool if agent.host_context else None | ||
| pool = agent._agent_pool |
There was a problem hiding this comment.
Accessing the private member _agent_pool directly violates encapsulation. While this change is part of removing the HostContext.pool escape hatch, it introduces another form of tight coupling. Consider adding a public property for _agent_pool on MessageNode if it's intended to be accessed by other modules, or refactoring this logic to avoid direct access to the agent's internal pool reference.
There was a problem hiding this comment.
Valid concern. This is a known trade-off from T11 (removing HostContext.pool).
Context: MessageNode.agent_pool property already exists but emits DeprecationWarning. T11 removed HostContext.pool (the escape hatch), so all access sites migrated to _agent_pool. The proper fix is to un-deprecate the agent_pool property now that HostContext.pool is gone — agent_pool is no longer a deprecated escape hatch but the only public interface.
This un-deprecation is out of scope for this PR. Tracked as follow-up tech debt. The current _agent_pool access is a pragmatic interim solution that avoids triggering deprecation warnings in production code.
…st, and inject_prompt changes
… as TODO - Add TODO(m4) comment at base_team.py:411 explaining _agent_pool usage - Add M4 tasks 14.8-14.11 for NodeContext.pool→host migration: - 14.8: Add input_provider to HostContext - 14.9: Migrate NodeContext.pool → NodeContext.host - 14.10: Move get_skill_instructions_for_node() to SkillsManager - 14.11: Audit protocol server _agent_pool refs
f68e745 to
a3af80d
Compare
…ure/pre-m4-protocol-cleanup
1 participant
Summary
This PR implements the pre-M4 protocol cleanup — 22 implementation tasks across 4 phases that eliminate ACP dual-path execution, remove legacy enums/fields, improve type safety, and wire up event system gaps before the M4 multi-config milestone.
What changed
Phase 1: ACP Execution Path Unification (T0-T6)
stream_events()andget_messages()methods with@runtime_checkableprotocols_stream_events()with thin delegate toACPTurn.execute()_run_stream_once(), removedhooks_fireddouble-fire guard (replaced with per-Turn_logged_toolsset)queue_prompt/inject_promptbranching — all agents now usesession_pool.followup()/steer()Phase 2: Legacy Field & API Cleanup (T7-T13)
_SessionContext→McpSessionContext(public), addedMCPManager.add_transport()initialize_mcp_servers()now uses MCPManager methods instead of mutating agent fields_mcp_snapshotand_session_connection_poolfieldsdeliver_feedback()to protocol (returnsbool), removed duck-typingpoolescape hatch fromHostContext, migrated all access sites to_agent_poolRunStatusenum withRunOutcome(COMPLETED/FAILED/CHECKPOINTED) +RunState(IDLE/RUNNING/DONE)session_controller.pyPhase 4: Type Safety & Code Quality (T14-T19)
type: ignore[attr-defined]inrun.pyby holding direct dimension refsCommChannelprotocol, replaced direct_replayingattribute access_channel_publishes_to_event_busisinstance check with protocol propertyhasattrpatterns withis_busyproperty andisinstancechecksexcept Exceptionclauses inACPTurnwith specific exception typesPhase 6: Event System Gaps (T20-T22)
MCPCapability.on_change()→EventProcessor→ SSE broadcastEventProcessornow emitsSessionStatusEvent(status="cancelled")for cancelled streams_handle_eventmethod, updated tests to useEventProcessordirectlyVerification
uv run ruff check src/— All checks passeduv run --no-group docs mypy src/— 0 errors in 607 source filesuv run pytest tests/integration/test_acp_streaming.py -m acp_snapshot— 2/2 snapshots passScope
In scope: Phases 1, 2, 4, 6 (22 tasks) + V10 snapshot baseline + F1-F4 verification
Out of scope (deferred):
subagent_display_moderemoval, M4 implementationCommits
35 commits, one per task + fix commits for type/lint issues discovered during verification.